[E2E][JF] Implemented JCM mDNS advertisement #38994
Open
+124
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andy31415
requested changes
May 15, 2025
There was a problem hiding this comment.
Please update the testing summary to contain commands tested and observed results. Verified using docs/guides/joint_fabric_guide.md instructions. seems to try to make it conventient to say manually tested.
However the whole intent of our policy is to make it inconvenient to just say "manually tested" to (strongly!) encourage writing automated tests. So in this case:
- write details on how you tested (generally what app you built, chip-tool or repl commands run and what were the observed results)
- explain why automated testing is impossible or what the schedule for adding automated tests is
andy31415
reviewed
May 15, 2025
bzbarsky-apple
requested changes
May 16, 2025
vijs
changed the title
|
PR #38994: Size comparison from 386263d to 7d65b8f Increases above 0.2%:
Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
doru91
reviewed
May 19, 2025
doru91
reviewed
May 19, 2025
|
General comment: I would say that this PR implements a subset of #38203 - the OJCW command. #38203 text also requires handling of some errors paths, so I would advise creating a separate sub-issue (see the "Create sub-issue" button) if those are scheduled for a follow up PR. What I would like to see in the testing section or added in the docs/guides/joint_fabric.md would be to show that |
examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.h
Outdated
Show resolved
Hide resolved
examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.h
Outdated
Show resolved
Hide resolved
|
PR #38994: Size comparison from bb6d92f to 4378967 Increases above 0.2%:
Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
vijs
force-pushed
the
feature/38544
branch
2 times, most recently
from
df9e098 to
1e18994
Compare
doru91
reviewed
May 21, 2025
You asked me to review this PR saying that all the comments have been addressed. However, most of the comments haven't been addressed. |
doru91
reviewed
May 29, 2025
|
PR #38994: Size comparison from f881f51 to 5bc0c49 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38994: Size comparison from 0a18b85 to ccfdc6d Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38994: Size comparison from 04a8c79 to f21465d Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
May 30, 2025
There was a problem hiding this comment.
What does this have to do with the mDNS advertisement?
I'm pretty confused now. I asked why CommissioningWindowManager had dead code and this PR adds not only code in CommissioningWindowManager to call SetJCM but also a whole bunch of other unrelated stuff.
What I would have expected here are PRs as follows:
- Core PR to enable advertising thew new keys, in lib/dnssd.
- A PR on top of that to add IsJointFabricEnabled to the provider API (no impl yet) and then update app/server/Dnssd.cpp.
- The changes to CommissioningWindowManager.
- All the other bits as desired (those are not core code at that point, and I don't need to look at them).
The comments above say "to simplify review", but this very much does not simplify review. There is no way I can approve this PR, because I am not going to read anything under examples/jf-admin-app, sorry.
There was a problem hiding this comment.
I initially planned to keep the PR small, but due to requests for unit tests and concerns about unused methods (potential dead code - #38994 (comment)), I've included the implementation of OpenJointCommissioningWindow() in this PR. Adding the OJCW command to the SDK and example JF apps allowed us to verify the accuracy of the mDNS advertisement and JF TXT key, remove several TODO comments, and include a functional Testing subsection in the PR description.
There was a problem hiding this comment.
Again: SetJCM did not need to exist until something used it, sure. And in general, there don't need to be any changes to CommissioningWindowManager to make the changes to lib/dnssd, and minimal changes to CommissioningWindowManager to make the changes to app/server/Dnssd.cpp.
There was a problem hiding this comment.
Hi Boris, this PR has been refactored to align with your recommendation. It now focuses solely on implementing mDNS advertisement, with the remaining changes to be addressed in subsequent PRs.
| @@ -31,6 +32,9 @@ class DLL_EXPORT CommissioningModeProvider | |||
| { | |||
| public: | |||
| virtual Dnssd::CommissioningMode GetCommissioningMode() const = 0; | |||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| virtual bool IsJointFabricEnabled() = 0; | |||
There was a problem hiding this comment.
Oh, and for the separate PR that will add this: having conditionally defined bits of vtables is a good recipe for mismatched vtable layouts and security bugs. Should generally be avoided.
Separately, this new virtual method needs documentation.
There was a problem hiding this comment.
Thanks for the feedback — I’ll address it in the next update.
There was a problem hiding this comment.
Thanks for the feedback. Based on your recommendation, this PR has been updated to focus exclusively on implementing mDNS advertisement. A follow-up PR will handle the OJCW functionalities and address the comments regarding unit tests. Closing out this thread for now.
|
PR #38994: Size comparison from 871b434 to 717b88c Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
doru91
reviewed
Jun 2, 2025
doru91
reviewed
Jun 2, 2025
doru91
reviewed
Jun 2, 2025
vijs
changed the title
|
PR #38994: Size comparison from 2eabb28 to 2d9ed24 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
|
PR #38994: Size comparison from 2eabb28 to 94a2e60 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
robertfarnum
approved these changes
Jun 4, 2025
There was a problem hiding this comment.
This seems completely unrelated to this PR. Please take it out and put it wherever it's supposed to go.
Comment on lines
+55
to
+58
| kAvailable, // This device is capable of acting as a Joint Fabric Administrator. | ||
| kAdministrator, // This device is acting as a Joint Fabric Administrator. | ||
| kAnchor, // This device is acting as a Joint Fabric Anchor Administrator. | ||
| kDatastore // This device is acting as a Joint Fabric Datastore. |
There was a problem hiding this comment.
These are not mutually exclusive in the spec, right? So shouldn't the values here be 1, 2, 4, 8, per spec?
I don't understand how you represent the spec "14" case otherwise.
| @@ -204,6 +215,15 @@ class CommissionAdvertisingParameters : public BaseAdvertisingParams<CommissionA | |||
| } | |||
| CommissioningMode GetCommissioningMode() const { return mCommissioningMode; } | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| CommissionAdvertisingParameters & SetJointFabricMode(std::optional<JointFabricMode> mode) | |||
There was a problem hiding this comment.
Should take optional<BitFlags<JointFabricMode>>, I would think, given that multiple flags can be set.
Because for example 14 (an example given in the spec) is not a valid value of JointFabricMode the way it's defined here.
At that point presumably you don't even need the optional, in that you can just use empty BitFlags to represent "none of the bits are set, don't list the key".
| @@ -878,6 +881,14 @@ FullQName AdvertiserMinMdns::GetCommissioningTxtEntries(const CommissionAdvertis | |||
| snprintf(txtPairingInstr, sizeof(txtPairingInstr), "PI=%s", *pairingInstruction); | |||
| txtFields[numTxtFields++] = txtPairingInstr; | |||
| } | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| if (const auto & jointFabricMode = params.GetJointFabricMode(); jointFabricMode.has_value()) | |||
There was a problem hiding this comment.
So here you could do (if GetJointFabricMode returns BitFlags<JointFabricMode>):
Suggested change
| if (const auto & jointFabricMode = params.GetJointFabricMode(); jointFabricMode.has_value()) | |
| if (const auto & jointFabricMode = params.GetJointFabricMode(); jointFabricMode.HasAny()) |
and then print jointFabricMode.Raw() into the string.
| @@ -198,6 +198,13 @@ CHIP_ERROR CopyTextRecordValue(char * buffer, size_t bufferLen, CommissioningMod | |||
| return CopyTextRecordValue(buffer, bufferLen, static_cast<uint16_t>(value)); | |||
| } | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| CHIP_ERROR CopyTextRecordValue(char * buffer, size_t bufferLen, JointFabricMode value) | |||
There was a problem hiding this comment.
Again, BitFlags to represent the flags, and Raw() to get an integer.
| @@ -161,6 +161,13 @@ uint8_t GetCommissioningMode(const ByteSpan & value) | |||
| return MakeU8FromAsciiDecimal(value); | |||
| } | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| uint8_t GetJointFabricMode(const ByteSpan & value) | |||
There was a problem hiding this comment.
Probably return BitFlags<JointFabricMode> here too? Like:
return BitFlags<JointFabricMode>(MakeU8FromAsciiDecimal(value));
or so.
| @@ -234,6 +235,9 @@ struct CommissionNodeData : public CommonResolutionData | |||
| char instanceName[Commission::kInstanceNameMaxLength + 1] = {}; | |||
| char deviceName[kMaxDeviceNameLen + 1] = {}; | |||
| char pairingInstruction[kMaxPairingInstructionLen + 1] = {}; | |||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| uint8_t jointFabricMode = 0; | |||
| @@ -48,6 +49,16 @@ enum class CommissioningMode | |||
| kEnabledEnhanced // Enhanced Commissioning Mode, CM=2 in DNS-SD key/value pairs | |||
| }; | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| enum class JointFabricMode | |||
There was a problem hiding this comment.
Suggested change
| enum class JointFabricMode | |
| enum class JointFabricMode : uint8_t |
|
@vijs for we have https://github.com/project-chip/connectedhomeip/blob/master/scripts/build_coverage.sh, although that covers a lot more than just the dnssd files (it is a full test run). What I need to get confidence in is "this code is tested" and to what extent it is. Please make the summary informative: this is not a "checklist item because we like bureaucracy" but rather we want to ensure that changes that go in the SDK are solid and well tested. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented JCM mDNS advertisement
Addresses #38544 & #39101
Testing
Verified using the below instructions